Add pure RSS fetch acceptance contract#39
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: Both current findings are still supported by the cumulative PR diff. First, 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🔴 [CRITICAL] Security in
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76fa6470bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if result["state"] == "accepted" and (result["accepted_row_count"] <= 0 or result["rejected_row_count"] != 0 or result["error_code"] is not None): | ||
| raise _fail("feed_result_invalid") |
There was a problem hiding this comment.
When a caller constructs a feed result instead of using classify_feed_payload (for example while recording external feed states), this validation allows kind="unknown" with state="accepted" and a positive accepted count. build_acceptance_status then treats that feed as successful and sets publication_complete/eligible_for_live_publication to true, so a canonical status can certify non-RSS/Atom content as publishable; accepted feed results should be constrained to rss or atom here.
Useful? React with 👍 / 👎.
| parsed = datetime.fromisoformat(normalized) | ||
| except (TypeError, ValueError): | ||
| return False | ||
| return parsed.tzinfo is not None and parsed.astimezone(timezone.utc) is not None |
There was a problem hiding this comment.
For feeds containing a syntactically valid timestamp that overflows when converted to UTC (for example 9999-12-31T23:59:59-23:59), fromisoformat/parsedate_to_datetime succeeds but this astimezone call raises OverflowError. That exception escapes classify_feed_payload, so a single malformed entry aborts classification instead of being counted as rejected and quarantining the feed.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def build_acceptance_status(feed_results: Iterable[Mapping[str, object]]) -> dict[str, object]: | ||
| if not isinstance(feed_results, list) or not feed_results: |
There was a problem hiding this comment.
The public signature accepts Iterable[Mapping[str, object]], but this guard rejects any non-list iterable before reading it. A natural integration such as passing a tuple or generator of classify_feed_payload(...) results will raise configured_feed_empty even when results are present; convert the iterable to a list before checking emptiness, or narrow the signature to list.
Useful? React with 👍 / 👎.
| root = ET.fromstring(payload) | ||
| except (ET.ParseError, UnicodeError, ValueError): | ||
| return _feed_result(feed_id, feed_url, "unknown", 0, 0, "failed", "xml_invalid") |
There was a problem hiding this comment.
For malformed XML that declares an unknown encoding, ET.fromstring raises LookupError rather than ET.ParseError/UnicodeError/ValueError. Because this exception is not caught, classify_feed_payload aborts instead of returning a failed xml_invalid feed result, so one bad payload can bypass the intended fail-closed acceptance status path.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df843262ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MAX_XML_NODES = 10000 | ||
| MAX_XML_TEXT_BYTES = 256 * 1024 | ||
| MAX_XML_ATTRIBUTES = 128 | ||
| _FORBIDDEN_DECLARATION = re.compile(rb"<!\s*(?:doctype|entity|element|attlist|notation)\b|\b(?:system|public)\b", re.IGNORECASE) |
There was a problem hiding this comment.
This pattern matches standalone public or system anywhere in the raw payload, not just inside <!DOCTYPE ... PUBLIC/SYSTEM ...> declarations. A valid political RSS item such as <title>Public hearing</title> is therefore classified as xml_forbidden_declaration, causing the feed to fail and preventing live publication even though the XML is well-formed RSS/Atom; scope those alternatives to declaration markup instead of scanning all content.
Useful? React with 👍 / 👎.
| raise _fail("feed_payload_invalid") | ||
| if len(payload) > MAX_XML_BYTES: | ||
| raise _fail("xml_oversize") | ||
| if _FORBIDDEN_DECLARATION.search(payload): |
There was a problem hiding this comment.
When a feed declares UTF-16/UTF-16LE, this raw byte scan never sees contiguous <!DOCTYPE or <!ENTITY tokens, but ET.fromstring below honors the encoding and expands the internal entity; a UTF-16 RSS payload with a forbidden DTD is currently returned as an accepted feed instead of xml_forbidden_declaration. Since the contract is fail-closed for forbidden declarations, normalize or restrict encodings before this check.
Useful? React with 👍 / 👎.
| if child.tag == f"{{{_ATOM}}}link" and child.attrib.get("href"): | ||
| return child.attrib["href"] |
There was a problem hiding this comment.
For Atom entries whose only link is href=" ", this condition treats the whitespace as a usable link, so row_valid counts the entry as accepted and build_acceptance_status can mark the feed publishable even though the downstream source_url would be blank/whitespace. Strip the attribute and require a non-empty value before returning it, matching how RSS text fields are handled.
Useful? React with 👍 / 👎.
| if state == "quarantined" and not error_code: | ||
| raise _fail("feed_result_invalid") |
There was a problem hiding this comment.
When external feed states are passed directly, this check only requires any error code for quarantined feeds, so a result like state="quarantined", error_code="zero_entries", and a positive accepted_row_count is accepted and serialized as canonical. That makes the strict status report accepted rows for a feed that claims to have zero entries, undermining the exact counter contract; reject quarantine reasons whose row counts are inconsistent with the reason.
Useful? React with 👍 / 👎.
1 participant
Summary
Pure fetch/content/status acceptance foundation for the existing PERT RSS producer. This PR does not modify workflow, publication, upload, QAR, Pages, or weekly artifact code.
Contract
publication_complete=trueValidation
python3 -m pytest -q— 122 passeduv sync --locked --extra test && uv run pytest -q— 122 passedpython3 -m compileall -q src scripts tests— passedgit diff --check— passedruffunavailable in environmentNon-scope
No workflow wiring, publication/upload changes, QAR/Pages/publisher, weekly artifact, new dependencies, secrets, or permissions.